Update 'restart connection' action to reset internal state#19971
Update 'restart connection' action to reset internal state#19971carlos-zamora wants to merge 1 commit intomainfrom
Conversation
DHowett
left a comment
DHowett
left a comment
There was a problem hiding this comment.
Hmmmm. I don't love how we have to send this all the way down through the RIS handler. However, it's also the only thing that knows the full gamut of things that need done to reset the terminal.
I'd love @j4james' opinion on the layering here.
If I had nits to pick, they would be:
ResetConnection should be named HardResetWithoutErase, clearBuffers should be named erase, BufferClear should be called Erase; later we can also add TermControl::HardReset which allows whoever's driving terminal to reset it with all the usual ceremony.
Also, why does TerminalCore need to reach into StateMachine? What state is stuck in stateMachine?
microsoft-github-policy-service
bot
added
the
Needs-Author-Feedback
|
Yeah, I'm a little uncomfortable with this partial hard reset concept. If we were using either a plain hard reset, or a plain soft reset that would be fine. It may not be exactly what we want, but they're at least well-defined concepts. This feels like we're hacking in the things we want to reset for a single use case. But maybe I'm being too picky. We had a similar discussion in PR #18437, which was never fully resolved. So I think we should at least try and make sure that what we're doing here is also going to work for that use case, so we can share the functionality if possible. I'm also a little worried about how this is going to interact with the conhost state. If we're resetting Windows Terminal, but not resetting conhost, are they not going to end up out of sync now? Do we not have a way to pass that reset through somehow? This is maybe less of an issue with the new conpty, but it could still affect shells that rely on the legacy console APIs. I might be overthinking things though. If you're doing a reset, the terminal is assumedly messed up already, so an imperfect fix is probably better than nothing. And assuming we do want to keep this |
3 participants
Summary of the Pull Request
I was becoming frustrated with getting in a state where a CLI app (i.e. Copilot CLI) enters some VT state (like mouse mode) then doesn't unset it when it accidentally exits. I normally use "Restart connection" to keep the buffer and connect again. Problem is, "restart connection" didn't actually reset the internal state! So I would type and get a bunch of lingering VT escape sequences.
Fixed that by updating the action to now also reset the VT state. Most of this PR is plumbing that through the layers. AdaptDispatch is where the real change actually happens:
_hardResetCore(bool)to be able to filter out the logic where the buffer is cleared (I don't want that part)HardReset()-->_hardResetCore(true)HardResetWithoutBufferClear()-->_hardResetCore(false)I decided to add to the existing behavior rather than adding a new action or action arg. That said, I'm very open to making changes. Just let me know. 😊
Validation Steps Performed
Write-Host -NoNewline "e[?1003he[?1006h"